Fix 3 d blox external spec#10107
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the 3dblox parser and logic to handle external file definitions (DEF, Verilog, SDC) more robustly by treating them as lists and flagging multiple entries as errors. It also adds logic to track DEF file reading status to prevent redundant processing. The review feedback recommends improving property creation safety, using idiomatic vector assignment, and ensuring consistent error logging for multiple file entries across all parser sections where checks are currently missing.
| chiplet.external.def_file.c_str(), | ||
| chip, | ||
| /*issue_callback*/ false); | ||
| odb::dbBoolProperty::create(chip, "def_file_read", true); |
There was a problem hiding this comment.
It is safer to check if the property already exists before creating it, especially since createChiplet might be called on an existing chip that already has this property set. This prevents potential errors or duplicate property creation.
if (odb::dbProperty::find(chip, "def_file_read") == nullptr) {
odb::dbBoolProperty::create(chip, "def_file_read", true);
}| std::vector<odb::dbLib*> search_libs; | ||
| for (odb::dbLib* lib : db_->getLibs()) { | ||
| search_libs.push_back(lib); | ||
| } | ||
| def_reader.readChip( | ||
| search_libs, chip_inst.external.def_file.c_str(), chip, false); |
There was a problem hiding this comment.
Per the general rules, use vector::assign() with iterators to copy elements from a dbSet to a std::vector instead of a manual loop. Additionally, adding the /*issue_callback*/ comment to the readChip call improves readability and maintains consistency with other parts of the codebase.
References
- To copy elements from a dbSet to a std::vector, use the vector::assign() method with iterators from the dbSet, as direct assignment is not supported.
| if (!verilog_files.empty()) { | ||
| chiplet.external.verilog_file = resolvePath(verilog_files[0]); | ||
| } |
There was a problem hiding this comment.
Add a check for multiple verilog_file entries and log it as an error. In this repository, error logs are terminal, ensuring the user is notified of the unsupported configuration before the program exits.
if (verilog_files.size() > 1) {
logError(
"Multiple verilog_file entries for a single chiplet are currently "
"unsupported.");
}
if (!verilog_files.empty()) {
chiplet.external.verilog_file = resolvePath(verilog_files[0]);
}References
- Unexpected failures should be logged as an error, not a warning.
- Calls to logger_->error are expected to terminate the program, so additional error handling like returning an error code or throwing an exception is not necessary.
| if (!verilog_files.empty()) { | ||
| external.verilog_file = resolvePath(verilog_files[0]); | ||
| } |
There was a problem hiding this comment.
Add a check for multiple verilog_file entries for consistency. This should be logged as an error, which is terminal per repository guidelines.
| if (!verilog_files.empty()) { | |
| external.verilog_file = resolvePath(verilog_files[0]); | |
| } | |
| if (verilog_files.size() > 1) { | |
| logError( | |
| "Multiple verilog_file entries for a single chiplet are currently " | |
| "unsupported."); | |
| } | |
| if (!verilog_files.empty()) { | |
| external.verilog_file = resolvePath(verilog_files[0]); | |
| } |
References
- Unexpected failures should be logged as an error, not a warning.
- Calls to logger_->error are expected to terminate the program, so additional error handling like returning an error code or throwing an exception is not necessary.
| if (!sdc_files.empty()) { | ||
| external.sdc_file = resolvePath(sdc_files[0]); | ||
| } |
There was a problem hiding this comment.
Add a check for multiple sdc_file entries and log an error if found. Note that error logging will terminate the program execution.
| if (!sdc_files.empty()) { | |
| external.sdc_file = resolvePath(sdc_files[0]); | |
| } | |
| if (sdc_files.size() > 1) { | |
| logError( | |
| "Multiple sdc_file entries for a single chiplet are currently " | |
| "unsupported."); | |
| } | |
| if (!sdc_files.empty()) { | |
| external.sdc_file = resolvePath(sdc_files[0]); | |
| } |
References
- Unexpected failures should be logged as an error, not a warning.
- Calls to logger_->error are expected to terminate the program, so additional error handling like returning an error code or throwing an exception is not necessary.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (external_node["verilog_file"]) { | ||
| extractValue(external_node, "verilog_file", external.verilog_file); | ||
| external.verilog_file = resolvePath(external.verilog_file); | ||
| std::vector<std::string> verilog_files; |
There was a problem hiding this comment.
warning: no header providing "std::vector" is directly included [misc-include-cleaner]
src/odb/src/3dblox/dbxParser.cpp:10:
+ #include <vector>
|
clang-tidy review says "All clean, LGTM! 👍" |
| if (!verilog_files.empty()) { | ||
| chiplet.external.verilog_file = resolvePath(verilog_files[0]); | ||
| } |
|
clang-tidy review says "All clean, LGTM! 👍" |
…ipletInst Address review feedback on The-OpenROAD-Project#10107 (osamahammad21: "please address") asking that the cardinality check applied to verilog_file in dbvParser.cpp and to def_file in dbxParser.cpp be extended to the remaining external fields for ChipletInst. Matches the behaviour requested by the 3DBlox external spec tracked in The-OpenROAD-Project#10078. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
…ect#10078) Per the 3DBlox v3.0 spec, every argument inside an external block must be a YAML list of strings (with wildcard expansion). Parse the single-cardinality external keys (verilog_file/sdc_file/def_file in .3dbx; DEF_file/verilog_file in .3dbv) through a shared extractSinglePathFromList helper that expands wildcards via resolvePaths and rejects more than one resolved file. Emit these keys as YAML lists on write for round-trip consistency. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
openroad-ci
force-pushed
the
fix-3DBlox-external-spec
branch
from
dbafce3 to
a8dcfe0
Compare
3 participants
Fix the issue #10078